Resolve native templates per container instead of through mutable statics - #573
Resolve native templates per container instead of through mutable statics#573lagergren wants to merge 8 commits into
Conversation
Native templates are built per container: NativeContainer constructs a complete set for every instance, and a single JVM can build several. Each template carries an owner, so a lookup has to answer with the template that belongs to the container doing the asking. NativeTemplates is the one place that answers that question. Templates that are addressable by name in the container's own registry - the name follows from the class, numbers.xFloat64 backs numbers.Float64 - are resolved lazily on first use. Templates that implement a composite type declared by another template, and so have no name of their own, are published by their declaring template from registerNativeTemplates(), which runs after construction. registerNativeTemplate() is split in two: the existing method now also claims the template's class, while registerAuxiliaryTemplate() only adds a type mapping. xRTComponentTemplate uses the latter, since it registers a second instance of its own class for RTMultiMethodTemplate and must not displace the template registered for its own structure. No behaviour change yet: the INSTANCE statics are still in place and still what the runtime reads.
Every native template published itself into a process-global mutable static from inside its own constructor - 139 `INSTANCE = this` sites, none of the fields volatile or final. Two things were wrong with that. The write is a this-escape: a partially constructed template becomes reachable JVM-wide before its constructor finishes. And the field is last-writer-wins across containers: templates are built per container, a single JVM can build several (the interpreter connector does), so the container built last silently took the statics over from every container built before it. That is broken in series, with no concurrency at all. Build container A, then container B, and code acting for A reads B's template - and through it B's container, pool and type system. ProxyComposition.getTemplate() returned Proxy.INSTANCE, so a composition belonging to A handed out a template owned by B; xRTFunction.makeAsyncNativeHandle pulled the container for an async call straight out of the static. All 139 assignments and all 143 declarations are gone. Lookups now go through the container's NativeTemplates table, reached from whatever the call site already has - a container, a frame, a template, or a composition. Where a static factory had no owner to work from, it takes one: xInt64/xChar/xUInt8/ xNibble/xString.makeHandle, xRTFunction.makeAsyncNativeHandle and the xRTSignature ensure* cluster all now take the container or frame that is asking. FullyBoundHandle.NO_OP, which needed a container to construct but is only ever compared by identity, becomes a lazily created singleton. `this == INSTANCE` becomes isNativeInstance(X.class). The class has to be named: the test that matters is whether this template is the native instance of the class that *declares* the method, and xVar extends xRef, so keying on getClass() would let a subclass pass a gate that was written for its base. INCEPTION_CLASS in Identity/xRef/xVar/xTuple/xService was the same defect in the same constructors - a NativeRebaseConstant over the container's own ClassConstant, kept in a static. It is now a final instance field. For xRef and xVar the getter asks the container for its own Ref/Var template rather than reading `this`, because the templates that inherit it - xFuture, xLazy, xAtomic, xInject - are not themselves rebased and never populate the field; reading `this` there produced a null inception constant and broke handle creation well outside the unit tests. Behaviour is unchanged for a single container, including one oddity worth naming: xRTDelegate registers a nibble delegate twice, and INSTANCE therefore held the second instance while the type registry held the first. The table keeps last-writer-wins so that stays true. NativeTemplatePerContainerTest builds two containers in series and asserts that a composition resolves the template of its own container. On master: container A (built first) must resolve its own Proxy template, but got one owned by a different container ==> expected: NativeContainer@57167ccb but was: NativeContainer@37753b69 The same assertion passes for container B, which is the last-writer-wins signature: whoever was built last is the only one the statics still serve.
With lookups going through the container's table, the flag no longer selects
anything in 137 of the 143 native templates - the `if (fInstance)` blocks it
guarded are gone. It cannot be removed, though, and it is worth saying why
before someone tries.
Six templates still read it, and xService is the one that settles it. It is
constructed both as the rebased base template over the `Service` interface and
as the template for every concrete service class, and only the first may build
a NativeRebaseConstant - that constructor asserts its argument is an interface.
Dropping the guard fails during bootstrap:
java.lang.AssertionError
at org.xvm.asm.constants.NativeRebaseConstant.<init>(NativeRebaseConstant.java:28)
at org.xvm.runtime.template.xService.<init>(xService.java:58)
at org.xvm.runtime.template._native.crypto.xRTKeyStore.<init>(xRTKeyStore.java:86)
The table cannot answer the question either, and that is by design: it is
populated after a template finishes constructing, which is exactly the property
that stopped templates escaping mid-construction. So the flag stays, uniformly -
the reflective loader in NativeContainer looks up one constructor shape for
every scanned template, and Container.getTemplate(IdentityConstant) passes it
explicitly for the seven formats it instantiates.
The migration left its call sites spelling out the path to the owner. Asking
for one template read:
frame.f_context.f_container.nativeTemplates().get(xListMap.class)
Four hops to name one template, at the exact sites this change exists to make
clear about ownership. Two field hops go through another object's internals,
and the two-step table lookup was ceremony: of the native-template lookups on
this branch, all but one were a single get.
Frame gains container(), and Container and Frame gain a nativeTemplate(clz)
shorthand, so the same site reads frame.nativeTemplate(xListMap.class).
That makes three ways to reach the table, which is one too many to leave
undocumented, so each now says which to reach for:
- NativeTemplates.of(container|frame|template|composition) is the adapter.
Its job is normalising the four inputs, and ClassTemplate and
TypeComposition are the reason it has to exist - they have no lookup
method of their own and are not meant to grow one. 24 call sites: 21
through a composition, 2 through a template, 1 through a container.
- Container.nativeTemplates() is the table itself, for work that is not a
single lookup - registering a template is the one such caller.
- Container.nativeTemplate(clz) / Frame.nativeTemplate(clz) is the shorthand
for the common case.
ClassTemplate deliberately does not get a nativeTemplate() of its own;
isNativeInstance() goes through f_container instead, which also names the
owner at the point the identity comparison is made.
Only the sites this branch already touches are converted. On lines this branch
introduces, X.f_context.f_container goes 89 -> 1 (the survivor is a
ServiceHandle, which has no container()), and nativeTemplates().get(...) goes
256 -> 1 (the survivor is Container.nativeTemplate's own body). The ~200
pre-existing f_context.f_container sites elsewhere in the runtime are left
alone; converting those is a separate mechanical change.
No behaviour change: the end-to-end run is still byte-identical to master.
The name was a fossil. "fInstance" meant "assign yourself to INSTANCE", and
that mechanism is gone. What survives reads absurd - 151 parameter
declarations, one `if`, six real users - so a reader's first conclusion is
that it is dead and should be deleted. A commit message arguing otherwise is a
worse fix than a name that does not raise the question.
fBaseTemplate over fRebase, which was the other candidate: five of the six
users spend the flag on a rebased inception identity, so fRebase describes
them well, but the sixth does not. xRTConnector passes false to its super and
uses its own flag to build a process-wide user-agent string - nothing there is
rebased. Across all 151 declarations the flag means one thing, "this is the
container's base template for this class", and five consumers happen to turn
that into a rebase. fBaseTemplate is true at every site; fRebase would be a
fossil of a different kind at the sixth.
The reasoning for keeping the flag at all, which has not changed:
It cannot be removed. xService is constructed both as the rebased base
template over the `Service` interface and as the template for every concrete
service class, and only the first may build a NativeRebaseConstant - that
constructor asserts its argument is an interface. Dropping the guard fails
during bootstrap:
java.lang.AssertionError
at org.xvm.asm.constants.NativeRebaseConstant.<init>(NativeRebaseConstant.java:28)
at org.xvm.runtime.template.xService.<init>(xService.java:58)
at org.xvm.runtime.template._native.crypto.xRTKeyStore.<init>(xRTKeyStore.java:86)
The native template table cannot answer the question either, and that is by
design: it is populated after a template finishes constructing, which is
exactly the property that stopped templates escaping mid-construction.
Nor can the parameter be dropped from the 137 templates that ignore it. The
reflective loader in NativeContainer looks up one constructor shape for every
scanned template, and Container.getTemplate(IdentityConstant) passes it
explicitly for the seven formats it instantiates. That uniformity is
load-bearing.
Pure rename; no behaviour change.
getPropertyCapacity divides an array length by two, so the (long) cast in front of it is inert: the result is smaller than the int it started from, and it widens on the way into makeHandle(Frame, long) regardless. The two sibling delegates need theirs. LongBasedDelegate multiplies the length by up to 64 and BitBasedDelegate shifts it left by three, and both overflow as int. Sitting next to two casts that are load-bearing is presumably how this one survived.
|
Made a mistake. Will reopen later. |
|
I completely disagree with the analysis. The "fInstance" argument is passed as "true" only during the native container initialization, which is by design a single-threaded action. There is a good point regarding My review decision: close; do not apply |
|
I reviewed |
…iner The tests here built two native containers in series to show that the earlier one still resolved its own templates. That argued the weaker of this change's two cases: it depends on a container topology that is disputed, and if that topology is ruled out the defect it demonstrates is unreachable - so the test could never settle anything. It also gated on compiled XDK output, which in the javatools build is downstream, so both of those tests silently assume-skipped and had never run. What this change actually establishes needs no container at all: a template is reached through its container's table rather than a JVM-global field. That is a property of the compiled classes, so read them. A reviewer of 139 mechanical sites can read a handful, recognise the pattern, and have the scan guarantee the other 135 did not deviate - including a site that kept the old shape under a different spelling, which reading source text would miss. Two tests remain, both of which run everywhere with no assumption: - the template tree declares no INSTANCE static and no code in it reads one, across all 318 compiled template classes - componentNameOf maps template classes to the component names the table resolves through Red on master, where the scan reports the INSTANCE statics it finds; green here. The javadoc names the statics this change does NOT touch (xPackage.LIST_MAP_TEMPLATE, xEnum.RANGE_TEMPLATE, xArray.MUTABILITY and others) so the test pins what was done rather than failing on work nobody claimed to have finished. NativeTemplates.resolvedNames() is the reason the table was wanted: asking which templates a container has pulled in is one call where it was previously not expressible at all. Its test needs a live container, so it belongs in the xdk build where the distribution exists.
|
Rewrote the tests in this PR after re-reading them. Two problems with what was there, both mine. They argued the weaker of this change's two cases. The tests built two native containers in series and checked the first still resolved its own templates. That depends on a container topology you've said goes against the design assumptions — and if that topology is ruled out, the defect those tests demonstrate is unreachable, so they could never settle anything. Meanwhile the case that needs no such agreement went untested: 139 copy-pasted mutable statics became one container-owned table. And they never ran. They gated on compiled XDK output via What's here now is two tests, both reading javatools' compiled classes — not XTC modules, nothing from the XDK.
The reason for reading class files rather than source is your review time. You read four of the 139 sites, recognise the pattern, and the scan guarantees the other 135 didn't deviate — including a site that kept the old shape under a different spelling, which text-matching would miss. It turns a wide, shallow diff from a reading task into one file plus a proof. Two things I want to be straight about. The scan keys on the name
|
Trimming this class to what runs without a container took ownerlessFactoryPathsStillWork with it. That was wrong: it pins two NPEs this PR introduced and then fixed - xRTType.makeForeignHandle, whose callers pass no frame, and Utils.translate, which turns a native throwable into a handle with a null frame. Both paths used to read a process-global template and so never needed an owner; they now take one from the composition they build against, and nothing else covers them. It builds one container and says nothing about how many a host may create, so it does not carry the topology claim the two deleted tests did. Its repository helper is not read-through: with it on, LinkedRepository answers a hit in a later repository by cloning the module into the first, and the first here is a read-only build output directory - the store fails and a module that was found comes back null. The checkout walk also accepts ".git" as a file, which is what it is in a linked worktree. 3 tests, 0 skipped.
|
Correcting myself on two points in the comment above. "They never ran" was wrong. I checked by restoring the old test file and running it in this branch's tree: all four tests ran, none skipped. What is true is narrower — they gate on I removed a test I should have kept. Trimming the class to what runs without a container also removed Its repository helper is also no longer read-through. With read-through on, The description is updated for the commit count, the diffstat and the test numbers, which had all gone stale. |
2 participants
The defect
Native templates published themselves into mutable
public staticfields from their constructors:139 sites did this. 196 sites read the result. None of the fields were
volatileorfinal.Templates are constructed per container (
NativeContainer:172-175), and a JVM can build several —InterpreterConnector:36doesnew NativeContainer(runtime, repository), and so does every embedding host and several tests.It is wrong in series, with no concurrency involved
Every
Connectorbuilds its own root:InterpreterConnector's constructor isnew Runtime()plusnew NativeContainer(...), andRunnerbuilds a Connector per run. A host that runs several modules in one JVM therefore has several native containers alive at once — the Gradle plugin's direct mode does exactly this, in one cached classloader.All ~139 native templates are constructed by a native container (
newInstance(this, structClass, Boolean.TRUE)), so each new root overwrites the statics. Two native containers are unrelated — neither is an ancestor or descendant of the other — so code acting on root A's behalf then reads a template owned by root B: wrong container, wrong pool, wrong type system.To be precise about what is not the problem: nested containers under a single root never construct native templates, and
getTemplate(String)resolves throughgetNativeContainer(), so under one rootINSTANCEholds an ancestor-owned template and reading it is legitimate. This bug needs a second root.The clearest case is
Proxy.makeAsyncNativeHandle, which doesnew AsyncHandle(INSTANCE.f_container, method): it takes aContainerout of a JVM-global mutable static — and with more than one root, that container can belong to an entirely different runtime.Note that the reachability sweep cannot catch this: it deliberately does not enumerate
staticroots. That is a reason to delete the static, not to trust a clean sweep.xEnum.initNative()also branches onif (this == INSTANCE), so which object won the assignment decides whether native rebase initialisation runs at all. That is behaviour, not caching.Add threads and it is additionally a
this-escape publishing a partially-constructed object to a shared static with no safe publication.The change
Template lookup moves into one container-owned table (
NativeTemplates), reached from the container that is asking rather than from a static. The dangerous pattern is expressed once, in one reviewable place, instead of being pasted 139 times.Zero
INSTANCE = thisand zero mutablepublic static … INSTANCEdeclarations remain. Net −75 lines (187 files, +1639/−1714) despite adding the module.Seven commits, each building independently: the table; the migration; a note recording why the
fInstanceconstructor flag has to stay (it does not become redundant, contrary to my first assumption); two readability passes (container accessors, and renaming that flag tofBaseTemplate); a dead cast; and the test rework described below.Verification
./gradlew buildgreen;:javatools:test351 tests, 0 failures, 40 skips (the standing set).xcc+xecon a real module: output byte-identical to master.A review pass over my own change caught two regressions I had introduced, both from a scripted rewrite threading a container or frame that master deliberately leaves null: an NPE on
xRTType.makeHandle's foreign-handle path (three callers passnullintentionally), and an NPE on every native-throwable translation inxException, becauseUtils.translatepasses a null frame. Both are fixed, folded into the commits that introduced them, and pinned byownerlessFactoryPathsStillWork.That pass also found composition caches I had bound to the first caller's container — which could be a short-lived nested container pinned forever in a static — where master bound them to the long-lived native container. Restored.
Readability: say which container is being asked
Two accessors, in their own commit, because the migration otherwise made the diff harder to read than the code it replaced. Call sites had become:
Four hops to name one template — and when the lookup is four hops through another object's fields, a reader cannot see whose table is being asked, which is the question this PR exists to make answerable.
Frame.container()— three lines; master had no such accessor.Container.nativeTemplate(Class<T>)andFrame.nativeTemplate(Class<T>)— a shorthand for the case that is 91% of all uses.On the lines this PR introduces:
f_context.f_containerwent 89 → 1, andnativeTemplates().get(...)went 256 → 1. Both survivors are deliberate — one chain starts from aServiceHandle, which has nocontainer(), and the other isContainer.nativeTemplate's own body.NativeTemplates.of(...)keeps all four overloads; they are layered rather than redundant, and each now says so in its javadoc.of(TypeComposition)alone has 21 call sites, and neither it norof(ClassTemplate)can be replaced by the shorthand, because those types have no owner accessor of their own.ClassTemplatedeliberately did not gain anativeTemplate(...)—isNativeInstancenow readsthis == f_container.nativeTemplate(clz), which names the owner at the identity comparison instead of hiding it.The ~198 pre-existing
f_context.f_containersites elsewhere in the tree are untouched. That sweep is mechanical and obvious, but it does not belong inside a 181-file diff.fInstancerenamed tofBaseTemplateThe flag used to mean "assign yourself to
INSTANCE". That mechanism is gone, so the name was a fossil pointing at a removed mechanism — and with 151 declarations against 6 real consumers, a reader's first conclusion is that it is dead.It is not. Removing it was attempted, and fails during bootstrap:
xServiceis constructed both as the rebased base template over theServiceinterface and as the template for every concrete service class, and only the first may build aNativeRebaseConstant, whose constructor asserts its argument is an interface. The table cannot answer the question either, and that is by design — it is populated after a template finishes constructing, which is precisely the property that stops templates escaping mid-construction.fBaseTemplaterather thanfRebase, because five consumers turn it into a rebase but the sixth does not:xRTConnectorpassesfalseto its super and uses its own flag to build a process-wide user-agent string. "This is the container's base template for this class" is true at all 151 sites; "rebase" is not.What this does NOT fix — see #572
This removes the
INSTANCEtemplate cache. It does not end last-writer-wins across containers, because a large population of owner-bearing values is still held in mutablepublic staticfields assigned from per-containerinitNative():xObject.CLASS,xString.EMPTY_STRING/EMPTY_ARRAY,xBoolean.TRUE/FALSE,xNullable.NULL,xArray's*_ARRAY_CLZcompositions,xRTDelegate.DELEGATES, and others.Two places that leaks through a public API even after this change:
xString.makeHandle(Container, char[])returns the globalEMPTY_STRINGfor a zero-length array, ignoring the container it was handed;xArray.makeBitArrayHandleand friends derive their owner from a staticTypeComposition.Those are pre-existing and out of scope here, but they mean the guarantee this change provides is narrower than its name suggests, and I would rather say so than let the name carry more weight than the code. Filed as #572.